Skip to content

fix(worker): Fix issue with stale permissions#1215

Merged
brendan-kellam merged 4 commits into
mainfrom
bkellam/fix-SOU-1174
May 21, 2026
Merged

fix(worker): Fix issue with stale permissions#1215
brendan-kellam merged 4 commits into
mainfrom
bkellam/fix-SOU-1174

Conversation

@brendan-kellam
Copy link
Copy Markdown
Contributor

@brendan-kellam brendan-kellam commented May 21, 2026

Summary by CodeRabbit

  • Bug Fixes

    • More reliable handling of non-2xx API responses across Bitbucket and other Git providers, improving 404/401/403 detection and recovery.
    • Account permission sync now clears inaccessible repositories when authentication or token-refresh errors occur, preventing stale access after failed syncs.
  • Refactor

    • Unified HTTP error handling and authorization classification across integrations for consistent failure behavior.
  • Tests

    • Added tests validating error classification and middleware behavior.
  • Changelog

    • Noted fix preventing stale repo permissions.

Review Change Stack

@github-actions

This comment has been minimized.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Walkthrough

Adds error-normalization utilities, defines/registers Bitbucket middleware that throws on non-2xx responses, updates Bitbucket call sites to rely on thrown errors and numeric e?.status checks, and makes account permission sync fail-closed by clearing accessibleRepos on authorization or refresh failures.

Changes

Error handling and fail-closed permission sync

Layer / File(s) Summary
Error classification utilities
packages/backend/src/errors.ts
New module exports isUnauthorized and isForbidden predicates that extract HTTP status codes from direct .status fields, wrapped errors, and nested cause.response.status shapes to classify 401 and 403 errors across multiple client libraries.
Bitbucket middleware and client registration
packages/backend/src/bitbucket.ts
Adds Middleware type import and defines throwOnHttpError middleware that converts non-2xx responses into thrown Error objects with attached .status fields; registers middleware on both Bitbucket Cloud and Server API clients.
Bitbucket Cloud API call updates
packages/backend/src/bitbucket.ts
Updates workspace, project, single-repo, and permission pagination to destructure only { data } from responses; changes 404 checks to e?.status === 404; removes manual { data, error } handling because middleware now throws.
Bitbucket Server API call updates
packages/backend/src/bitbucket.ts
Updates Server project, single-repo, and all-repos pagination to destructure only { data } from responses; changes 404 checks to e?.status === 404; removes manual error-field branching in favor of middleware-thrown errors.
Bitbucket Server auth probing refactor
packages/backend/src/bitbucket.ts
Removes exported isBitbucketServerUserAuthenticated helper and replaces its control flow with a direct /rest/api/1.0/profile/recent/repos probe so middleware-thrown auth errors propagate before pagination.
Permission syncer error handling and fail-closed logic
packages/backend/src/ee/accountPermissionSyncer.ts
Introduces RefreshTokenError wrapper; wraps ensureFreshAccountToken in try/catch to convert refresh failures; wraps syncAccountPermissions in try/catch to detect unauthorized/forbidden/refresh errors, clear the account's accessibleRepos, and rethrow to mark the job failed.
Test coverage for error classification and middleware
packages/backend/src/errors.test.ts
Vitest suite validating isUnauthorized and isForbidden classification against Octokit/Gitbeaker/synthetic Bitbucket errors; verifies throwOnHttpError contract (no throw on 2xx, throws on non-2xx with .status field); tests precedence of direct .status over nested cause.response.status.
Changelog
CHANGELOG.md
Adds an "Unreleased" Fixed bullet noting repo permissions no longer go stale when auth or token refresh errors occur.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • sourcebot-dev/sourcebot#1000: Both PRs modify packages/backend/src/ee/accountPermissionSyncer.ts around ensureFreshAccountToken and refresh-failure handling.
  • sourcebot-dev/sourcebot#998: The main PR refactors Bitbucket Server authenticated-user detection and overlaps with the deletion of isBitbucketServerUserAuthenticated.
  • sourcebot-dev/sourcebot#600: Related prior work on the account-scoped permission syncer and job wiring.

Suggested reviewers

  • msukkari
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(worker): Fix issue with stale permissions' directly and clearly summarizes the main change: addressing stale repository permissions through improved error handling in the account permission syncer.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bkellam/fix-SOU-1174

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/backend/src/bitbucket.ts`:
- Around line 55-58: The current errorconstruction reads the full response with
response.clone().text() and interpolates it into the Error.message via
Object.assign(new Error(`Bitbucket API ${response.status}: ${body}`), { status:
response.status }), which can leak payloads and create huge logs; change this to
keep the Error.message concise (e.g. `Bitbucket API ${response.status}`), attach
the full body to a non-message property (e.g. error.body or error.responseBody)
and if needed store a truncated/redacted excerpt (limit to a small length)
rather than embedding the entire body, while still preserving the status
property; update the throw site where Object.assign and new Error are used so
logs capture the short message and developers can inspect error.body if
necessary.

In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 196-199: The fail-closed branch checking
isUnauthorized/isForbidden/RefreshTokenError is bypassed because
syncAccountPermissions() wraps GitHub 401/403 into a plain Error; update the
error handling in syncAccountPermissions to either preserve the original HTTP
status when creating the wrapped error (e.g., copy status/statusCode onto the
new Error) or avoid wrapping and rethrow the original error after adding
context, so isUnauthorized/isForbidden will detect revoked GitHub tokens; ensure
the change touches the error creation/throwing site and keeps
isUnauthorized/isForbidden/RefreshTokenError checks working as-is.
- Around line 188-211: The try/catch around this.syncAccountPermissions
currently swallows non-auth errors so runJob()/onJobCompleted() mark the job
COMPLETED even when sync failed; modify the catch to rethrow any error that is
not handled by the auth checks by adding a throw error after the
unauthorized/forbidden/RefreshTokenError handling (or remove the catch and only
handle auth errors explicitly), ensuring syncAccountPermissions failures
propagate to runJob() and trigger onJobFailed().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3701deaa-f10f-42e0-a483-ebef83618bd2

📥 Commits

Reviewing files that changed from the base of the PR and between 79b3767 and f7c31be.

📒 Files selected for processing (4)
  • packages/backend/src/bitbucket.ts
  • packages/backend/src/ee/accountPermissionSyncer.ts
  • packages/backend/src/errors.test.ts
  • packages/backend/src/errors.ts

Comment thread packages/backend/src/bitbucket.ts Outdated
Comment thread packages/backend/src/ee/accountPermissionSyncer.ts
Comment thread packages/backend/src/ee/accountPermissionSyncer.ts
msukkari
msukkari previously approved these changes May 21, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/backend/src/ee/accountPermissionSyncer.ts (2)

223-237: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't treat every refresh-path failure as token revocation.

The catch at Line 235 rewrites every ensureFreshAccountToken() failure into RefreshTokenError, so transient issues in that path will also hit the fail-closed delete in runJob(). That can wipe valid permissions for accounts whose OAuth grant is still fine.

Only convert refresh responses that prove the grant is invalid to RefreshTokenError; let other refresh-path failures bubble as ordinary job failures.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/backend/src/ee/accountPermissionSyncer.ts` around lines 223 - 237,
The current catch around ensureFreshAccountToken(account, this.db)
indiscriminately wraps all errors as RefreshTokenError, causing transient
failures to trigger permission deletion in runJob; change this so you only
convert to RefreshTokenError when the underlying error clearly indicates an
invalid grant (e.g., inspect error.response?.data?.error === 'invalid_grant',
error.message contains 'invalid_grant', or a specific OAuth error code), and
rethrow the original error for all other cases so ordinary job failures bubble
up; update the catch in accountPermissionSyncer.ts accordingly (referencing
ensureFreshAccountToken and RefreshTokenError and how runJob treats
RefreshTokenError).

191-211: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail closed on scope-loss errors too.

This branch only clears permissions for 401/403/RefreshTokenError, but syncAccountPermissions() still throws plain Error when a GitHub token lacks repo or a GitLab token lacks read_api. Those are permanent authorization failures too, so the job will still leave stale accessibleRepos behind.

Consider rethrowing those scope checks as a typed auth error here, or classifying them in this branch as fail-closed conditions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/backend/src/ee/accountPermissionSyncer.ts` around lines 191 - 211,
The catch block in accountPermissionSyncer currently only clears accessibleRepos
for isUnauthorized, isForbidden, or RefreshTokenError; extend the fail-closed
logic to also treat token-scope errors from syncAccountPermissions as permanent
authorization failures by including the scope-check error classification here
(or by mapping those scope errors to a typed auth error earlier). Update the
condition that checks isUnauthorized/isForbidden/RefreshTokenError to also check
the scope-loss predicate (or the new AuthScopeError type) so that
this.db.account.update({ where: { id: account.id }, data: { accessibleRepos: {
deleteMany: {} } } }) runs for scope-loss cases as well before rethrowing the
error. Ensure you reference the same symbols: isUnauthorized, isForbidden,
RefreshTokenError, syncAccountPermissions, accessibleRepos and preserve
rethrowing the original error after clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@packages/backend/src/ee/accountPermissionSyncer.ts`:
- Around line 223-237: The current catch around ensureFreshAccountToken(account,
this.db) indiscriminately wraps all errors as RefreshTokenError, causing
transient failures to trigger permission deletion in runJob; change this so you
only convert to RefreshTokenError when the underlying error clearly indicates an
invalid grant (e.g., inspect error.response?.data?.error === 'invalid_grant',
error.message contains 'invalid_grant', or a specific OAuth error code), and
rethrow the original error for all other cases so ordinary job failures bubble
up; update the catch in accountPermissionSyncer.ts accordingly (referencing
ensureFreshAccountToken and RefreshTokenError and how runJob treats
RefreshTokenError).
- Around line 191-211: The catch block in accountPermissionSyncer currently only
clears accessibleRepos for isUnauthorized, isForbidden, or RefreshTokenError;
extend the fail-closed logic to also treat token-scope errors from
syncAccountPermissions as permanent authorization failures by including the
scope-check error classification here (or by mapping those scope errors to a
typed auth error earlier). Update the condition that checks
isUnauthorized/isForbidden/RefreshTokenError to also check the scope-loss
predicate (or the new AuthScopeError type) so that this.db.account.update({
where: { id: account.id }, data: { accessibleRepos: { deleteMany: {} } } }) runs
for scope-loss cases as well before rethrowing the error. Ensure you reference
the same symbols: isUnauthorized, isForbidden, RefreshTokenError,
syncAccountPermissions, accessibleRepos and preserve rethrowing the original
error after clearing.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1a3cd1d3-1806-4ca8-ae2e-be75dda34819

📥 Commits

Reviewing files that changed from the base of the PR and between a6e39a7 and f5cc244.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • packages/backend/src/ee/accountPermissionSyncer.ts
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md

@brendan-kellam brendan-kellam merged commit 8e527c8 into main May 21, 2026
10 checks passed
@brendan-kellam brendan-kellam deleted the bkellam/fix-SOU-1174 branch May 21, 2026 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants